-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ignore Debian-specific error around urdfdom_headers #614
ignore Debian-specific error around urdfdom_headers #614
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #614 +/- ##
==========================================
- Coverage 58.82% 57.90% -0.92%
==========================================
Files 91 132 +41
Lines 8623 10665 +2042
Branches 0 956 +956
==========================================
+ Hits 5072 6174 +1102
- Misses 3551 4444 +893
- Partials 0 47 +47 ☔ View full report in Codecov by Sentry. |
Why do you use python3-rosdep2 in first place? It is known that this is incompatible with ROS's rosdep. |
Of course it is incompatible with OpenRobotics packages, but it's a very valid basis to build ROS on Debian as well as Ubuntu 22.04 and newer and I'm very much for supporting the Debian Science Robotics initiative so more packages might eventually migrate to proper Debian packages. |
I am not convinced merging this. Why isn't the Debian Science Robotics initiative simply using the
I cannot comprehend that argument. Obviously, the Debian packages provide ROS1 support currently. If so, rosdep should report packages compatible to ROS1. If the Debian packages switch to ROS2 in future, it will be the right time to reintroduce the Finally, disabling dependency checking here for all packages is a relaxation of the linting rules, which I don't like. |
Just answering for the Debian part here.
Debian only allows packages from Debian as a dependency so we need rosdep to build other ROS packages in Debian.
I don't agree here and I offered multiple times to help making the OSRF package compatible with Debian but in the end I don't see a problem here.
The Debian package supports ROS1 and 2. |
There are numerous issues on the web complaining about the incompatibility of rosdep versions provided by OpenRobotics and Debian. Not seeing a problem with this, is weird.
Surely rosdep itself supports both versions. But that's not the point. Your specific config exports a certain set of package keys, which is valid only for a specific subset of ROS distributions. To support varying key sets in different distros, OpenRobotics provides a separate key file for each distro, which you obviously don't do. |
I agree that there is a problem but to my understanding this can only be solved on the OSRF side and not on the Debian side as explained here: https://discourse.ros.org/t/upstream-packages-increasingly-becoming-a-problem/10902/2
It provides a set of package keys for a specific Debian distribution.
That seems unrelated, for example the keys are fine for building ROS 2 as I did for example here: https://github.com/jspricke/ros-two-packages just that not all packages are in Debian (but there are a number of ROS 2 packages in Debian already). |
Ok. I agree. Probably, you need to release your own rosdep package with your own key list to have a consistent setup within the Debian distro.
I didn't know that. Isn't mixing ROS1 and ROS2 packages in the same distro (and both installing to /usr) becoming chaotic? I think, quite often, both packages will install exactly the same binaries? I guess you resolve that by declaring corresponding The problem faced by @v4hn is that the MTC package he wants to build in the Debian distro originates from the Noetic distro, which doesn't declare the According to @v4hn you refrained from removing the I think, having separate apt repositories and separate install locations for individual ROS distros, is a much cleaner approach to separate them. For example, this allows to install multiple distros (e.g. 1&2) side-by-side. I'm afraid we cannot solve that issue. I suggest ignoring the catkin_lint issue, @v4hn (but w/o the technical help suggested in this PR as this relaxes the linting too much). |
Till now we did not upload packages that conflict (and I would like to avoid using
+1. |
Btw. afair it is possible to override the keys provided by Debian by either modifying the files in |
The latest catkin_lint release (1.6.24) supports ignores with instance restrictions, i.e.,
|
7cbb199
to
acfe53c
Compare
Updated to include @roehling newly supported syntax. Thank you so much ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that solution, because it is restrive enough.
Without the patch: $ catkin_lint rviz_marker_tools/ rviz_marker_tools: package.xml: error: missing build_depend on 'urdfdom_headers' requires catkin_lint 1.6.24 for successful error suppression.
acfe53c
to
6c95679
Compare
Without the patch I see this building with the Debian ROS packages:
Backstory for the error in catkin_lint: The python3-rosdep package defines this key although ROS does not do that from indigo to noetic anymore. However, ROS2 has a package - and thus a rosdep key - with that name again. So dropping the key entry from the Debian package is no real option assuming eventual support for ros2, but I also do not see another nice way for
catkin_lint
to support this check better. Thus, I discussed with @jspricke and we decided it might be best to nolint the error here.Note that this prevents simple commits as catkin_lint without errors is a hard prerequisite for our
pre-commit rules
and I would like to keep it that way.@roehling It seems
catkin_lint
does not supportignore_once missing_depend
, which is why I had to disable the warning for the whole package. I seem to recall some issue about just this years ago, but I cannot find anything related. If relevant, I can also create an upstream issue of course.